Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: add check to remove old kube-rbac-proxy container in modelregistry operator, fixes RHOAIENG-15328 #1341

Merged
merged 3 commits into from
Nov 8, 2024
Merged

fix: add check to remove old kube-rbac-proxy container in modelregistry operator, fixes RHOAIENG-15328 #1341

merged 3 commits into from
Nov 8, 2024

Conversation

dhirajsb
Copy link
Contributor

@dhirajsb dhirajsb commented Nov 5, 2024 •
edited
Loading

Description

Add check to remove old kube-rbac-proxy container in modelregistry operator

Fixes RHOAIENG-15328

How Has This Been Tested?

Tested locally by creating a DSC with MR, and manually adding a kube-rbac-proxy container with the following commands:

# create a patch file with kube-rbac-proxy container
cat - >> rbac-container.yaml << EOF
spec:
  template:
    spec:
      containers:
      - args:
        - --secure-listen-address=0.0.0.0:8443
        - --upstream=http://127.0.0.1:8080/
        - --logtostderr=true
        - --v=0
        - --tls-cert-file=/etc/server-cert/tls.crt
        - --tls-private-key-file=/etc/server-cert/tls.key
        image: gcr.io/kubebuilder/kube-rbac-proxy:v0.14.1
        name: kube-rbac-proxy
        ports:
        - containerPort: 8443
          name: https
          protocol: TCP
        resources:
          limits:
            cpu: 500m
            memory: 128Mi
          requests:
            cpu: 5m
            memory: 64Mi
        securityContext:
          allowPrivilegeEscalation: false
          capabilities:
            drop:
            - ALL
        volumeMounts:
        - mountPath: /etc/server-cert
          name: server-cert
          readOnly: true
EOF
# patch the modelregistry operator deployment
oc patch deployment -n opendatahub model-registry-operator-controller-manager --patch-file rbac-container.yaml
# restart the odh operator, it doesn't detect that a resource it created was modified!!!
oc delete pod -n opendatahub-operator-system -l name=opendatahub-operator
watch oc get pods -n opendatahub

Watch the number of pods in the model-registry-operator-controller-manager deployment, which should change to 1 after the kube-rbac-proxy container is removed by this patch.

Screenshot or short clip

Merge criteria

  • You have read the contributors guide.
  • Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
  • Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@dhirajsb
fix: add check to remove old kube-rbac-proxy container in modelregist…
abac782 
…ry operator, fixes RHOAIENG-15328

Signed-off-by: Dhiraj Bokde <[email protected]>
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Nov 5, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign steventobin for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dhirajsb
Copy link
Contributor Author

dhirajsb commented Nov 5, 2024

@lburgazzoli @zdtsw @VaishnaviHire please review

@codecov Codecov
Copy link

codecov bot commented Nov 5, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (incubation@656d53e). Learn more about missing BASE report.

Additional details and impacted files 
@@              Coverage Diff              @@
##             incubation    #1341   +/-   ##
=============================================
  Coverage              ?   19.08%           
=============================================
  Files                 ?       30           
  Lines                 ?     3369           
  Branches              ?        0           
=============================================
  Hits                  ?      643           
  Misses                ?     2657           
  Partials              ?       69

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zdtsw zdtsw added the odh-2.21 label Nov 6, 2024
@dhirajsb
Copy link
Contributor Author

dhirajsb commented Nov 7, 2024

your changes lgtm @zdtsw 👍

@dhirajsb
Copy link
Contributor Author

dhirajsb commented Nov 8, 2024

@zdtsw @VaishnaviHire please merge if it looks good

@zdtsw zdtsw merged commit 92251e4 into opendatahub-io:incubation Nov 8, 2024
9 of 10 checks passed
@zdtsw
Copy link
Member

zdtsw commented Nov 8, 2024

@zdtsw @VaishnaviHire please merge if it looks good

sorry, forgot i have the permission to merge this one..... thought need someone else to approve it :D

zdtsw pushed a commit to zdtsw-forking/opendatahub-operator that referenced this pull request Nov 8, 2024
@dhirajsb @zdtsw
fix: add check to remove old kube-rbac-proxy container in modelregist…
a5771c8 
…ry operator, fixes RHOAIENG-15328 (opendatahub-io#1341)

* fix: add check to remove old kube-rbac-proxy container in modelregistry operator, fixes RHOAIENG-15328

Signed-off-by: Dhiraj Bokde <[email protected]>

* update: remove check in ModelReg code, add it to upgrade logic

Signed-off-by: Wen Zhou <[email protected]>

---------

Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Wen Zhou <[email protected]>
Co-authored-by: Wen Zhou <[email protected]>
(cherry picked from commit 92251e4)
@zdtsw zdtsw mentioned this pull request Nov 8, 2024
Merged
5 tasks
openshift-merge-bot bot pushed a commit that referenced this pull request Nov 11, 2024
@zdtsw @dhirajsb
[sync]: change from incubation to rhoai + dummy change (#1357)
b6124ef 
* fix: ensure input CAbundle always end with a newline (#1339)

* fix: ensure input CAbundle always end with a newline

- missing newline will cause problem when inject data into configmap
- this happens when user use kustomize with their own CA for DSCI, it set to use |- not |
- fix lint

Signed-off-by: Wen Zhou <[email protected]>

* update: better way to add newline

Signed-off-by: Wen Zhou <[email protected]>

---------

Signed-off-by: Wen Zhou <[email protected]>
(cherry picked from commit 656d53e)

* fix: add check to remove old kube-rbac-proxy container in modelregistry operator, fixes RHOAIENG-15328 (#1341)

* fix: add check to remove old kube-rbac-proxy container in modelregistry operator, fixes RHOAIENG-15328

Signed-off-by: Dhiraj Bokde <[email protected]>

* update: remove check in ModelReg code, add it to upgrade logic

Signed-off-by: Wen Zhou <[email protected]>

---------

Signed-off-by: Dhiraj Bokde <[email protected]>
Signed-off-by: Wen Zhou <[email protected]>
Co-authored-by: Wen Zhou <[email protected]>
(cherry picked from commit 92251e4)

* fix: cherry-pick error

- remove new logger
- dummy: request from devops to verify auto-sync

Signed-off-by: Wen Zhou <[email protected]>

---------

Signed-off-by: Wen Zhou <[email protected]>
Co-authored-by: Dhiraj Bokde <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VaishnaviHire VaishnaviHire VaishnaviHire left review comments

@zdtsw zdtsw zdtsw left review comments

@mattmahoneyrh mattmahoneyrh Awaiting requested review from mattmahoneyrh

Assignees
No one assigned
Labels
odh-2.21 priority/high
Projects
ODH Platform Planning
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dhirajsb @zdtsw @VaishnaviHire